fix(triggers): pr-conflict-detected fires on opened + reopened, not only synchronize#1252
Conversation
…nly synchronize ucho/PR #226 was created by aaight at 2026-05-02T16:51:32 already CONFLICTING against `dev`. resolve-conflicts never fired. The PR sat open with merge conflicts and no auto-resolution; the user had to manually intervene. Trigger config IS enabled for ucho: resolve-conflicts scm:pr-conflict-detected yes Root cause: `pr-conflict-detected.matches()` at `src/triggers/github/pr-conflict-detected.ts:32-42` hard-coded `payload.action !== 'synchronize'`, rejecting `opened` events. Since PR #226 only fired a `pull_request.opened` event (no commits pushed since), the matcher returned `false` and `handle()` never ran. The handler body — gates on cascade persona + base branch, retries on `mergeable === null`, dispatches when `mergeable === false` — never got the chance. The existing test at lines 80-88 explicitly codified the bug (`does not match non-synchronize action` with `action: 'opened'`). Fix: widen the matcher to accept `opened`, `reopened`, and `synchronize` — the three actions GitHub emits when the PR head is the candidate state we should check for mergeability. `closed`, `edited`, `labeled`, `assigned`, `ready_for_review` correctly stay rejected. Why the handler body needs no changes: the `mergeable === null` retry loop already handles GitHub's async mergeability computation (most prominent on `opened`); cascade-persona + base-branch gates and the `gateAttemptLimit` remain untouched. The cost of the widening is one extra getPR API call per opened/reopened cascade-impl PR — small and bounded by the persona gate. Tests: - Replaced inverted `does not match non-synchronize action` (was the bug pin) with `matches opened action (PR #226 regression pin)` asserting true. - Added `matches reopened action`. - Replaced single `does not match closed action` with an `it.each` over `closed`, `edited`, `labeled`, `unlabeled`, `assigned`, `ready_for_review` to confirm we narrowed the rejection set, not widened carelessly. Out of scope (follow-up): periodic mergeability re-check for PRs whose base branch advanced and created a new conflict without any per-PR event. Real concern for long-lived PRs, but the simpler `opened` fix solves PR #226's case. Defer. Verification: - `npx vitest run --project unit-triggers tests/unit/triggers/pr-conflict-detected.test.ts` → 22/22. - `npx vitest run --project unit-triggers --project unit-api` → 2503/2503. - `npm run typecheck` + `npm run lint` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM - clean, minimal bug fix with solid root-cause analysis and well-structured tests. The matcher widening from synchronize-only to opened/reopened/synchronize is correct: all three are GitHub candidate-head-SHA signals, and the handler body (persona gate, base-branch gate, mergeability retry loop, attempt limit) already handles all three identically. The opened+reopened overlap with PROpenedTrigger is safe - they dispatch different agent types (resolve-conflicts vs review) and the concurrency lock is per-agent-type. Test coverage is thorough: positive pins for all three accepted actions (including the PR #226 regression pin), and it.each negative coverage across six rejected actions.
🕵️ claude-code · claude-opus-4-6 · run details
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Live regression — auto-resolve never fired on PRs that open with conflicts
ucho/PR #226 (`aaight`, opened 2026-05-02T16:51:32, base=`dev`, mergeable=CONFLICTING, mergeStateStatus=DIRTY) never had `resolve-conflicts` dispatched. The PR sat conflicting with no auto-resolution; user had to manually intervene.
Trigger config IS enabled for ucho:
```
Agent Event Enabled Parameters
resolve-conflicts scm:pr-conflict-detected yes -
```
So configuration is fine. Bug is in the matcher.
Root cause
`src/triggers/github/pr-conflict-detected.ts:32-42` hard-codes `payload.action !== 'synchronize'`:
```ts
matches(ctx: TriggerContext): boolean {
if (ctx.source !== 'github') return false;
if (!isGitHubPullRequestPayload(ctx.payload)) return false;
const payload = ctx.payload;
// Only trigger on synchronize events (when PR head is pushed/updated)
if (payload.action !== 'synchronize') return false;
return true;
}
```
PR #226 only fired `pull_request.opened` (no commits pushed since). Matcher returned `false` → `handle()` never invoked → handler's mergeability retry + dispatch path never ran. The PR is stuck conflicting until someone manually pushes a commit, which is exactly the friction we're supposed to be removing.
The existing test at lines 80-88 explicitly codified the bug (`does not match non-synchronize action` with `action: 'opened'`). This PR flips it.
The fix
One-line widen: accept `opened`, `reopened`, AND `synchronize`. All three are GitHub's "PR head is the candidate state" signals. `closed`, `edited`, `labeled`, `assigned`, `ready_for_review` correctly stay rejected.
```ts
if (
payload.action !== 'opened' &&
payload.action !== 'reopened' &&
payload.action !== 'synchronize'
) {
return false;
}
```
Why the handler body needs no changes
The handler at lines 44+ already covers all three event types correctly:
Cost of widening: one extra getPR API call per opened/reopened cascade-impl PR. Bounded by the cascade-persona gate.
Tests
Out of scope (intentionally — flag for follow-up)
Test plan
🤖 Generated with Claude Code